-
Notifications
You must be signed in to change notification settings - Fork 756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework validation, add queryset filter method #788
Rework validation, add queryset filter method #788
Conversation
a5c9091
to
1b3bcf7
Compare
Codecov Report
@@ Coverage Diff @@
## develop #788 +/- ##
===========================================
+ Coverage 97.81% 97.86% +0.04%
===========================================
Files 15 15
Lines 1145 1123 -22
===========================================
- Hits 1120 1099 -21
+ Misses 25 24 -1
Continue to review full report at Codecov.
|
@carltongibson, could you give this a brief look before 1.1? I'd like to add a deprecation note if the overall gist seems reasonable |
In short, the The only API-level change is that the
|
Yes. In general I like this.
|
Sweet. the only change necessary for 1.1 would be a deprecation note for the |
@rpkilby OK If you add that, we'll call 1.1 a wrap. Thanks! |
1b3bcf7
to
b98f254
Compare
8a8429d
to
5d54ebf
Compare
5d54ebf
to
ad68391
Compare
ad68391
to
9e6959b
Compare
- Remove 'FILTERS_STRICTNESS' setting. - Remove 'strict' option from FilterSet. - Add 'raise_exception' class attribute to DjangoFilterBackend. - Add 'strict' and 'get_strict' to FilterMixin.
9e6959b
to
b9f9d27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. This is great.
(I'll pick up the docs updates later on. It's not pressing.)
docs/ref/filterset.txt
Outdated
@@ -130,6 +130,26 @@ This is a map of model fields to filter classes with options:: | |||
Overriding ``FilterSet`` methods | |||
-------------------------------- | |||
|
|||
When overriding classmethods, calling ``super(MyFilterSet, cls)`` may result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These super
calls don't look right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What doesn't look right here? The example is intended to show how you would accidentally generate a NameError
.
@@ -159,4 +179,4 @@ filters for a model field, you can override ``filter_for_lookup()``. Ex:: | |||
return django_filters.DateRangeFilter, {} | |||
|
|||
# use default behavior otherwise | |||
return super(ProductFilter, cls).filter_for_lookup(f, lookup_type) | |||
return super().filter_for_lookup(f, lookup_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one does though.
* Extract validation and filtering from caching * Remove STRICTNESS, handle strictness in views - Remove 'FILTERS_STRICTNESS' setting. - Remove 'strict' option from FilterSet. - Add 'raise_exception' class attribute to DjangoFilterBackend. - Add 'strict' and 'get_strict' to FilterMixin. * Add 'is_valid()' and 'errors' to FilterSet API * Add FilterSet.get_form_class() * Remove strictness from tests * Drop FilterSet strict/together docs * Add note on overriding FilterSet classmethods * Add FilterSet API tests * Add tests for view strictness * Rework 'raw_validation' util * Update filter backend+tests
* Extract validation and filtering from caching * Remove STRICTNESS, handle strictness in views - Remove 'FILTERS_STRICTNESS' setting. - Remove 'strict' option from FilterSet. - Add 'raise_exception' class attribute to DjangoFilterBackend. - Add 'strict' and 'get_strict' to FilterMixin. * Add 'is_valid()' and 'errors' to FilterSet API * Add FilterSet.get_form_class() * Remove strictness from tests * Drop FilterSet strict/together docs * Add note on overriding FilterSet classmethods * Add FilterSet API tests * Add tests for view strictness * Rework 'raw_validation' util * Update filter backend+tests
Hello guys, as documentation is not ready yet, I would like to clarify how properly to override strictness for |
filterset = filter_class(request.query_params, queryset=queryset, request=request) | ||
if not filterset.is_valid() and self.raise_exception: | ||
raise utils.translate_validation(filterset.errors) | ||
return filterset.qs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I noticed that even if I do not want to raise exception here, then it is not raised, but queryset returned without filtering. I guess in case if invalid filterset and do not raise exception it should return qs.none()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It keeps the partial queryset, yet - I think this is tested for in https://github.com/carltongibson/django-filter/pull/788/files#diff-b18add7202661e9eacece0aded8e619bR301.
Override
|
Thanks @carltongibson , I just thought that it would be nice to have some kinda setting or one line setup. As I have to create and override |
The only thing you'd need to override is the backend's class StrictDjangoFilterBackend(django_filters.rest_framework.DjangoFilterBackend):
"""Return no results if the query doesn't validate."""
def filter_queryset(self, request, queryset, view):
try:
return super().filter_queryset(request, queryset, view):
except serializers.ValidationError:
return queryset.none() |
I'm guessing this wasn't intentional, given that it wasn't called out in the release notes or original PR. However, 2.0 is out in the wild now so provide a way for users to revert to the previous behavior, if they so choose. All of this should be integrated into a proper document, but that's a job for another day. Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 78febd1 ("Rework validation, add queryset filter method (carltongibson#788)")
Exceptions like ValidationError can include an optional 'params' argument that contains parameters to format the 'message' argument with. Handle these. Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 78febd1 ("Rework validation, add queryset filter method (carltongibson#788)")
Exceptions like ValidationError can include an optional 'params' argument that contains parameters to format the 'message' argument with. Handle these. Signed-off-by: Stephen Finucane <stephen@that.guru> Fixes: 78febd1 ("Rework validation, add queryset filter method (carltongibson#788)")
hey. I just want to know where can i find docs to properly comply strictness in view level now? |
Unfortunately, it's not really documented.
You can subclass the backend and set
In that case, you'd also need to override the |
@rpkilby This helped a lot! |
Can i set |
Not currently. Although, you could create a backend that does pull |
Hi @carltongibson Previously we had a test on the filters In particular, this is in contradiction with the docs, which states that
Is it really intended behaviour? We developed our API knowing empty filters would be handled this way. For sure, we can Subclass the class, define our own I think our use-case is a pretty common one and is not handled properly by this change. Also I think it is in contradiction with the docs. Thanks for your work, for the hopefully upcoming answer and I'll be glad to help if you wish so |
Hi @MRigal.
Yes - this did not allow django-filter/django_filters/filters.py Lines 139 to 141 in 9305753
Worth noting that the
That said, a filter subclass that overwrites
The docs are a little wonky here. It's not that it's not possible, it's that you wouldn't want to, since empty strings cannot be represented as bare values in a query string, as they are identical to empty form inputs.
I'm not completely following what you're doing here, but in general:
|
Hi @rpkilby Thanks for this quick and very well written answer. Our problem was actually smaller than I thought. We were actually calling the It was working before due to the double-check on the set, but the problem was on our Filter classes, as the work done there should have been paying more attention on which data comes in. Django-filters is actually working as expected, also not in contradiction with the docs and it is much better with this rewrite. Sorry for the mess and keep up with the good job! |
No worries, and thanks! And the docs may not be entirely incorrect, but they could be more clear as to what's going on. Also, we should probably mention checking against |
Brief update - I was actually wrong about django-filter/django_filters/filters.py Lines 757 to 761 in 1880430
So, the only issue would be custom |
This is an evolution of #783. In short, I think the validation/strictness behavior can be improved by moving it out of the FilterSet and into the view code. The current
.qs
implementation is somewhat difficult to extend, and is a little confusing given that it performs both form validation and queryset filtering.Overall changes:
.is_valid()
- proxiesform.is_valid()
.errors
- proxiesform.errors
.filter_queryset(qs)
- allows users to easily override queryset handling. Cached by.qs
.get_form_class()
- easier overriding of the form class on a per-instance basis, cached by.form
FilterSet
-level strictness handling is removed in favor of shifting it to the view layerRETURN_NO_RESULTS
/IGNORE
)RAISE_VALIDATION_ERROR
/IGNORE
)raw_validation
. Error details now include codes. However, these codes are only reachable viaexc.get_full_details()
, and do not automatically show up in the rendered error response.Additional thoughts:
forms.ValidationError
in the context of a Django view does not make any sense, especially given that none of the provided view code actually handles this exception. Raising an exception has been moved to the backend, where it uses the same utility code to raise thefilterset.errors
.FilterMixin.strict
option.FILTERS_STRICTNESS
setting is no longer necessary.Meta.together
handling byform.cleaned_data
TODO:
Determine how much deprecation warning needs to occur (if at all)no deprecation for strictnessFilterBackend
/FilterMixin
validation behaviorDocument new public FilterSet APIpostponing to docs overhaulAdditionally, this resolves #740 by extending #738. Inlining the original PR comment: